types: Change LM CDQ sz argument from u8 to u32.#1006
Conversation
|
The problem is that this is a stable API, which means all changes should be made with backward compatibility in mind. This change will break existing users which will update libnvme but not nvme-cli. The current way to handle this is to add padding (the new struct size needs to be multiple of the largest member, due to alignment constraints) and than add a new membber. In the function the arg type size is then check and the different versions used, e.g. 5ac91b7 ("types: Add ns-mgmt host software specified fields"). |
DanceDance
force-pushed
the
nvme-lm-cdq-args-sz-fix
branch
3 times, most recently
from
5be1982 to
d4b3835
Compare
|
Thanks, @igaw, for the explanation. Please take a look at the latest change. I did some manual testing as well, but with this approach we would need another change to nvme_cli to use the newly created sz_u32 as well. |
| @@ -989,6 +991,8 @@ struct nvme_lm_cdq_args { | |||
| __u16 cdqid; | |||
| __u8 sel; | |||
| __u8 sz; | |||
There was a problem hiding this comment.
The good news is that we don't promise source code backwards compatibility. That means you could rename this one and name the new member sz. Then we don't have to modify nvme-cli, only recompile it.
There was a problem hiding this comment.
That's a good point! I've updated the patch. I did test locally without modifying nvme_cli and it worked too.
DanceDance
force-pushed
the
nvme-lm-cdq-args-sz-fix
branch
from
d4b3835 to
c267988
Compare
To maintain backwards compatibility, the old sz renamed to sz_u8. According to NVMe 2.1 Base Specificaiton, Controller Data Queue Size (CDQSIZE) occupies bits 31:00 of CDW12, so it should be using u32. Signed-off-by: Dmitry Sherstoboev <[email protected]>
igaw
force-pushed
the
nvme-lm-cdq-args-sz-fix
branch
from
c267988 to
e1b1026
Compare
|
just renamed |
|
Thanks! |
2 participants
According to NVMe 2.1 Base Specificaiton, Controller Data Queue Size (CDQSIZE) occupies bits 31:00 of CDW12, so it should be using u32.
Note that in other places, like nvme-cli, the correct u32 is used: